Repository navigation
Make #[track_caller] async fn track the caller, not the poller/awaiter - #163396
theemathas wants to merge 8 commits into
Conversation
f21a754 to
7e01c99
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
7e01c99 to
c87bff0
Compare
| if is_in_trait_impl { | ||
| // Check if we need to "inherit" #[track_caller] from the trait definition. | ||
| let Some(trait_item_def_id) = | ||
| self.get_partial_res(node_id).and_then(|r| r.expect_full_res().opt_def_id()) | ||
| else { | ||
| self.dcx().span_delayed_bug(span, "could not resolve trait item being implemented"); | ||
| return false; | ||
| }; | ||
| return find_attr!(self.tcx, trait_item_def_id, TrackCaller(_)); | ||
| } |
There was a problem hiding this comment.
I don't understand what this is doing, but I'm doing the same thing as this existing code:
rust/compiler/rustc_ast_lowering/src/item.rs
Lines 1148 to 1155 in c1070d6
| /// | ||
| /// FIXME(async_fn_track_caller): What if the Location is stored inside a coroutine upvar? | ||
| fn resolve_tracked_call_location(body: &Body, inherited: Option<MirConst>) -> MirConst { |
There was a problem hiding this comment.
What is this, and do I need to modify it?
|
cc @rust-lang/miri Some changes occurred to the CTFE machinery Some changes occurred in compiler/rustc_attr_ir cc @jdonszelmann, @JonathanBrouwer Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri |
c87bff0 to
b14e6ed
Compare
This comment was marked as resolved.
This comment was marked as resolved.
b14e6ed to
e1d818f
Compare
This comment has been minimized.
This comment has been minimized.
e1d818f to
6fcdc09
Compare
|
I split up the implementation into multiple commits, hopefully making reviewing easier. (And also hopefully making it less confusing for me.) |
6fcdc09 to
473dbf0
Compare
#[track_caller] async fn track the caller, not the poller#[track_caller] async fn track the caller, not the poller/awaiter
473dbf0 to
57abba7
Compare
f857e66 to
b3015fc
Compare
This comment has been minimized.
This comment has been minimized.
|
Rebased, since #163746 was merged. In the process, the commit previously cherry-picked from there was removed. Here's a possibly more useful range-diff I'll address Ralf's comments later. |
…fJung consteval: Rework caller location handling Instead of walking up the call stack each time we need the caller location, we emulate the run time behavior of passing an implicit caller location argument in `#[track_caller]` functions. We record a representation of that argument in each frame. For inlined functions though, we still do a "stack walk" within that one frame. This PR is a prerequisite for rust-lang/rust#163396 Fixes rust-lang#5351
…ter. Part 1: AST lowering
…ter. Part 2: MIR building
…ter. Part 3: misc
b3015fc to
e4d0827
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
And rebased again after #163262 was merged. |
…ter. Part 4: codegen
…ter. Part 5: Miri
…ter. Part 6: Cranelift
e4d0827 to
fd39ca1
Compare
|
I've addressed @RalfJung's comments |
|
@RalfJung I've added a bunch of Miri tests, but the behavior doesn't make sense to me at all. Any suggestions on fixing these? 😅 |
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
What exactly doesn't make sense? What needs fixing? For the tests that involve the aliasing model I'd suggest switching to Tree Borrows, it has fewer unintuitive corner cases. |
9cdcb2d to
7bbcfe3
Compare
|
@RalfJung I've rewritten the aliasing test, and edited some stuff. Here are things I find confusing in the current state of the tests in
|
This comment has been minimized.
This comment has been minimized.
7bbcfe3 to
aa9ffe4
Compare
|
I don't know the MIR being run here, it is generated by the async lowering. You are asking me to spend a lot of time reverse engineering the exact steps that the auto-generated MIR is taking here. Is that worth it? Do you for some reason not want that code to have UB?
It looks like the caller location isn't read every single time the async fn is polled. It's apparently only read on the first poll and the third poll. I don't know much about the sync lowering so I can't tell you why that is the case. I don't understand the point of these tests, to be honest. Why should we have such fragile tests that probe implementation details of the generator lowerings? |
I might have misunderstood your intentions. I thought that you wanted me to have the miri implementation of track_caller match the run time implementation so that Miri's UB always matches whatever the run time implementation assumes. Making sure that this is the case presumably requires fragile tests that reflect what things are assumed by the implementation. Am I misunderstanding what I was supposed to do? |
|
A potential alternative is to, in miri, require that all caller locations inside |
|
I just figured out why the tests behaved so weirdly. It's because I did a dumb and made a typed copy of the |
View all comments
An LLM was used to locate relevant code, suggest ideas, and review the code. However, I manually wrote all code, and I take all responsibility for all code written and all decisions made.
Tracking issue (
async_fn_track_caller): #110011Related tracking issue (
closure_track_caller): #87417The first commit adds extensive tests for behavior touched in this PR. Commits 2-7 are the actual implementation, and also modification of the tests to reflect this new behavior.
This PR makes coroutines desugared from
#[track_caller] async fntrack the caller of the function, not the poller/awaiter of the coroutine. This is done as per T-lang's decision at #110011 (comment). The implementation modifies AST lowering to add an upvar to the coroutine, which stores the relevant caller location.In both codegen and in consteval/miri, we read the relevant upvar from the coroutine at the point in time where we need access to the
&Locationreference (either when we call thecaller_locationintrinsic, when we panic, or when we call another#[track_caller]function). We only dereference this reference as needed, when we actually need theLocationinformation.This PR changes the behavior of
async fn, but not ofasyncclosures, due to difficulties I've noted at #t-compiler/help > Help with `async_fn_track_caller` @ 💬This PR's implementation assumes that the desired behavior in #110011 (comment) is the behavior that I think makes the most sense.
This PR corrects the behavior of #163406 when the
async_fn_track_callerfeature is enabled, but does not give a warning when the feature is disabled (resulting in the pre-existing broken behavior).The tests pass on cranelift when I run
./x test ui --test-codegen-backend cranelift -- async-await/track-caller/panic-track-caller-no-unwind.rs. (Cranelift doesn't support unwinding.)r? compiler